Skip to content

Conversation

@AndreiDurlea
Copy link
Contributor

@AndreiDurlea AndreiDurlea commented Feb 10, 2026

@AndreiDurlea AndreiDurlea marked this pull request as draft February 10, 2026 19:16
@AndreiDurlea
Copy link
Contributor Author

@neatudarius is this ok to merge?

@coveralls
Copy link

coveralls commented Feb 10, 2026

Pull Request Test Coverage Report for Build 21914486666

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 99.038%

Files with Coverage Reduction New Missed Lines %
include/beman/optional/optional.hpp 2 98.99%
Totals Coverage Status
Change from base Build 21907778507: -0.1%
Covered Lines: 309
Relevant Lines: 312

💛 - Coveralls

// include/beman/optional/detail/stl_interfaces/iterator_interface.hpp -*-C++-*-
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

// Copyright (C) 2019 T. Zachary Laine
Copy link
Member

@neatudarius neatudarius Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contribution Andrei! It seems we havr an unexpected case here, I didn't anticipate these files need to be updated.

We need to remove these lines to be Beman Standard compliant with include/. I'm not sure if we can legally do this with the this header which was copied from @tzlaine . @steve-downey ?

v1: directly remove the copyright line
v2: move stl interfaces into a special whitelisted directory (e.g., external/, deps/)

@steve-downey
Copy link
Member

It's actually somewhat worse, it looks like the SPDX block at the top of the iterator headers is wrong.
This is essentially vendored in from @tzlaine 's iterator support proposal that he put on hold to reexamine after reflection.
I don't think it's a terrible problem with respect to the license. We should be clearer about the provenance and license.
Also I'll check on the symbol names and namespace. I wouldn't want this to collide with a different usage if the facility by accident.

1 similar comment
@steve-downey
Copy link
Member

It's actually somewhat worse, it looks like the SPDX block at the top of the iterator headers is wrong.
This is essentially vendored in from @tzlaine 's iterator support proposal that he put on hold to reexamine after reflection.
I don't think it's a terrible problem with respect to the license. We should be clearer about the provenance and license.
Also I'll check on the symbol names and namespace. I wouldn't want this to collide with a different usage if the facility by accident.

@neatudarius
Copy link
Member

It's actually somewhat worse, it looks like the SPDX block at the top of the iterator headers is wrong. This is essentially vendored in from @tzlaine 's iterator support proposal that he put on hold to reexamine after reflection. I don't think it's a terrible problem with respect to the license. We should be clearer about the provenance and license. Also I'll check on the symbol names and namespace. I wouldn't want this to collide with a different usage if the facility by accident.

Agree, we should remove SPDX lines from these 3 files.

I propose the following approach:

  1. Remove SPDX from these 3 files NOW. That can be a seperate PR attach to same issue - we can merge that ASAP.
  2. Add a way to ignore these files and NOT BE REPORTED by the tool.

===================================
I see 2 ways to ignore files: 1) having a special ignore folder or 2) having a beman-tidy config in your repo, with an ignore section.
v1) It's simpler to implement in the tools, but more inconvenient for the library developer (is it?).
v2) Generic approach, similar to all linters.

I propose to do #2, e.g., .beman-tidy.yml

ignored_paths:
    - include/beman/optional/detail/stl_interfaces/config.hpp
    - include/beman/optional/detail/stl_interfaces/fwd.hpp 
    - include/beman/optional/detail/stl_interfaces/iterator_interface.hpp
# or add support for regex
    - include/beman/optional/detail/stl_interfaces/  

@steve-downey @AndreiDurlea , what do you think?

@neatudarius neatudarius marked this pull request as ready for review February 11, 2026 15:21
@AndreiDurlea
Copy link
Contributor Author

AndreiDurlea commented Feb 11, 2026

I think approach 2 sounds right, I'll get to implementing it if everyone's on board with the idea.

@AndreiDurlea
Copy link
Contributor Author

  1. Remove SPDX from these 3 files NOW. That can be a seperate PR attach to same issue - we can merge that ASAP.

@neatudarius to clarify, I'm removing SPDX but reverting the commit that removed the Copyright notice. Is that ok?

@AndreiDurlea AndreiDurlea changed the title Apply file.copyright | early 2026 check Apply file.copyright from early 2026 check Feb 11, 2026
Copy link
Member

@neatudarius neatudarius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for spdx changes

@AndreiDurlea AndreiDurlea merged commit 4ed33a1 into bemanproject:main Feb 11, 2026
80 checks passed
@steve-downey
Copy link
Member

I think approach 2 sounds right, I'll get to implementing it if everyone's on board with the idea.

The ignore list sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants